Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

MmioPatterns optimisation #1607

Open
wants to merge 10 commits into
base: arith-dev
Choose a base branch
from
Open

MmioPatterns optimisation #1607

wants to merge 10 commits into from

Conversation

ahamlat
Copy link
Contributor

@ahamlat ahamlat commented Dec 2, 2024 •
edited
Loading

Following an issue we faced recently on sepolia on linea_generateConflatedTracesToFileV2, where tracing the batch of blocks take more than 30 seconds we noticed that MmioPatterns.updateTemporaryTargetRam method takes more than 2.5 seconds to execute (see profiling below).

This PR improves this method by using Java native calls (System.arraycopy) to copy byte arrays instead of using Tuweni library.

Before this PR

image image

With this PR
image

image

Currently the test takes around 25 seconds instead of 30 seconds

[ec2-user@ip-10-40-43-134 besu]$ time curl --location --request POST 'localhost:8545' --header 'Content-Type: application/json' --data-raw '{"jsonrpc": "2.0", "method": "linea_generateConflatedTracesToFileV2", "params": [{"startBlockNumber":6488125,"endBlockNumber":6488189, "expectedTracesEngineVersion": "0.8.0-rc6"}], "id": 1}'
{"jsonrpc":"2.0","id":1,"result":{"tracesEngineVersion":"0.8.0-rc6","conflatedTracesFileName":"/data/besu/conflated/6488125-6488189.conflated.0.8.0-rc6.lt"}}
real	0m24.473s
user	0m0.005s
sys	0m0.000s

@ahamlat
Improve updateTemporaryTargetRam.updateTemporaryTargetRam performance
d06727b 
Signed-off-by: Ameziane H <[email protected]>
@ahamlat
Remove unnecessary cast
521fc7b 
Signed-off-by: Ameziane H <[email protected]>
@ahamlat
Spotless
596d750 
Signed-off-by: Ameziane H <[email protected]>
@ahamlat
Fix index issue
7beaec4 
Signed-off-by: Ameziane H <[email protected]>
@ahamlat
Fix index issue
905f5b3 
Signed-off-by: Ameziane H <[email protected]>
@ahamlat
Fix index issue
8292ad6 
Signed-off-by: Ameziane H <[email protected]>
@ahamlat
Fix index issue
9dc21be 
Signed-off-by: Ameziane H <[email protected]>
FlorianHuc
FlorianHuc reviewed Dec 3, 2024
View reviewed changes
linea-constraints Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should not be updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why it was updated, it changed when I checkout the project. I thought it was expected.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just point to latest master of constraint and it'll be ok. The tracer is currently pointing to a commit that disables the MMIO (needed for the release)

arithmetization/src/main/java/net/consensys/linea/zktracer/module/mmio/MmioPatterns.java Outdated
@@ -182,6 +182,23 @@ public static Bytes16 excision(

public static void updateTemporaryTargetRam(
MmuData mmuData, final long targetLimbOffsetToUpdate, final Bytes16 newLimb) {

int limbStart = (int) (LLARGE * targetLimbOffsetToUpdate);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could add a simple unit test to validate the method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, @letypequividelespoubelles, I will need some help on creating a unit test as we don't have one before. I don't visualize yet the real use case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe @OlivierBBB, as Francois is OOO currently.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is needed when a (target) limb is updated in two times by a MMU instructions. It's the case for example for MMU instructions generating *TwoTarget MMIO instructions.
We already have some memory tests that triggers those MMIO inst.

Maybe this is the failing unit tests (not much network at the hospital, can't open the test output), but I guess it's MMIO.ram_excision or MMIO.*_to_ram_two_targets constraints that are failing.

@ahamlat
remove dead code
e2eae6e 
Signed-off-by: Ameziane H <[email protected]>
letypequividelespoubelles
letypequividelespoubelles requested changes Dec 3, 2024
View reviewed changes
arithmetization/src/main/java/net/consensys/linea/zktracer/module/mmio/MmioPatterns.java Outdated
int sliceSize = Math.max(0, originalRam.length - limbEnd);

int size = limbStart + newLimb.size() + sliceSize;
byte[] updatedRam = new byte[size];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here the updated ram size should be the same as the original one ram

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this method you're modifying is doing : you just want to modify a 16 bytes limb the midldle of your memory so :

  • you wipe everything before the limb (you could keep it, but as you don't need it anymore, tiu can put 0s instead)
  • you modify your 16 bytes limb
  • you keep unchanged everything that's after the unmodified limb

Not sure of what you're doing, I'm on mobile with low network

Copy link
Contributor Author

@ahamlat ahamlat Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here the updated ram size should be the same as the original one ram

In my current implementation, it is the case, but any way, we need to follow this logic. The total size is the sum of the parts' sizes that we're putting together.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure of what you're doing, I'm on mobile with low network

I'm doing the same work, but this implementation is faster as you can see in the flamegraphs

@ahamlat
Copy link
Contributor Author

ahamlat commented Dec 4, 2024

Using the new approach we discussed today during the meeting is even better. So the idea is to use an implementation of MutableByte on targetRamBytes and set the new limb at the right offset.
The method doesn't appear in the profiling any more

image

@ahamlat
Copy link
Contributor Author

ahamlat commented Dec 4, 2024

I don't understand why linea-constraints are modified, I haven't modify those files myself, maybe an encoding issue as I'm working on Mac.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FlorianHuc FlorianHuc FlorianHuc left review comments

@letypequividelespoubelles letypequividelespoubelles letypequividelespoubelles requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ahamlat @FlorianHuc @letypequividelespoubelles